Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: get rid of github.com/nspcc-dev/neo-go/pkg/io dependency #89

Merged
merged 4 commits into from
Feb 9, 2024

Conversation

AnnaShaleva
Copy link
Member

Implement custom payloads serialization using gob package. A part of #2.

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: 65 lines in your changes are missing coverage. Please review.

Comparison is base (6d59675) 74.94% compared to head (c8df9ff) 73.18%.

Files Patch % Lines
payload/recovery_message.go 28.57% 33 Missing and 7 partials ⚠️
config.go 0.00% 5 Missing and 1 partial ⚠️
payload/consensus_message.go 75.00% 3 Missing and 2 partials ⚠️
payload/message.go 86.66% 2 Missing and 2 partials ⚠️
payload/change_view.go 80.00% 1 Missing and 1 partial ⚠️
payload/commit.go 80.00% 1 Missing and 1 partial ⚠️
payload/prepare_request.go 87.50% 1 Missing and 1 partial ⚠️
payload/prepare_response.go 80.00% 1 Missing and 1 partial ⚠️
payload/recovery_request.go 80.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   74.94%   73.18%   -1.76%     
==========================================
  Files          24       24              
  Lines        1313     1365      +52     
==========================================
+ Hits          984      999      +15     
- Misses        275      298      +23     
- Partials       54       68      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AnnaShaleva
Copy link
Member Author

OK, need to fix linter.

@AnnaShaleva AnnaShaleva force-pushed the untie-neogo branch 2 times, most recently from a33583f to 15b3f97 Compare February 9, 2024 14:00
@AnnaShaleva
Copy link
Member Author

Ready for review.

merkle/merkle_tree_test.go Show resolved Hide resolved
signature [signatureSize]byte
}
type (
commit struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only use them in tests, what if we're to move them into _test.go files? It'd avoid importing encoding/gob for non-test builds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case is that we use them not only in tests, we also use them as default payloads for dBFT. So it won't be possible to easily integrate dBFT with Geth node without these default payloads.

Copy link
Member Author

@AnnaShaleva AnnaShaleva Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single-node Geth implementation used them before custom payloads implementation was added. Who knows, maybe one day we'll integrate dBFT with some other blockchain.

payload/commit.go Show resolved Hide resolved
w.WriteU16LE(p.validatorIndex)
w.WriteVarBytes(data)
// EncodeBinary implements Serializable interface.
func (p Payload) EncodeBinary(w *gob.Encoder) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this structure as well, who needs it exported?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref. #89 (comment), DBFT needs it as a default payload implementation:

dbft/config.go

Lines 115 to 121 in 6d59675

NewConsensusPayload: defaultNewConsensusPayload,
NewPrepareRequest: payload.NewPrepareRequest,
NewPrepareResponse: payload.NewPrepareResponse,
NewChangeView: payload.NewChangeView,
NewCommit: payload.NewCommit,
NewRecoveryRequest: payload.NewRecoveryRequest,
NewRecoveryMessage: payload.NewRecoveryMessage,
.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these "default implementations" make any real sense other than for tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how about some real intermediate implementation, like we had in Geth?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The node implemented in https://github.com/bane-labs/go-ethereum/pull/28 just won't work without these defaults. Now we have all the payloads properly implemented, but I think it's useful to have some intermediate working prototype (with default payloads) while integrating dBFT into other blockchain.

payload/prepare_response.go Show resolved Hide resolved
Implement custom payloads serialization using gob package. A part of #2.

Signed-off-by: Anna Shaleva <[email protected]>
Some issues are found in the old files by the updated linter job.

Signed-off-by: Anna Shaleva <[email protected]>
Make method comments start with the corresponding method name.

Signed-off-by: Anna Shaleva <[email protected]>
@roman-khimov roman-khimov merged commit 92d9aec into master Feb 9, 2024
5 of 7 checks passed
@roman-khimov roman-khimov deleted the untie-neogo branch February 9, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants